fix(connection): fail fast on unrecoverable connect errors (#4026) - #4166
Open
sean-kim05 wants to merge 1 commit into
Open
fix(connection): fail fast on unrecoverable connect errors (#4026)#4166sean-kim05 wants to merge 1 commit into
sean-kim05 wants to merge 1 commit into
Conversation
connect() wrapped the whole connect+handshake in the retry policy and re-wrapped every OSError into a generic ConnectionError, so definitively unrecoverable failures such as a missing Unix socket path (ENOENT) were retried DEFAULT_RETRY_COUNT times with full exponential backoff. Pass an is_retryable predicate from connect() that returns False for a conservative set of unrecoverable errnos (currently ENOENT), and preserve the original OSError as the exception cause (raise ConnectionError(...) from e) so the retry policy can classify it. Transient errors like ECONNREFUSED during server startup remain retryable, so the intentional handshake-retry behavior is unchanged. Mirrored across the sync and async connection stacks, with tests in both.
Collaborator
|
Hi @sean-kim05, thank you for your contribution! I'll review it shortly. |
Sanjays2402
reviewed
Jul 18, 2026
|
|
||
| # OS-level errno values for which reconnecting can never succeed, so | ||
| # there is no point spending the retry/backoff budget on them. | ||
| _UNRETRYABLE_CONNECT_ERRNOS = frozenset({errno.ENOENT}) |
Contributor
There was a problem hiding this comment.
ENOENT is transient for a Unix socket while the server is starting: the path does not exist until the server binds it. This makes an explicitly configured retry stop after one attempt in that common startup race; should this behavior be opt-in rather than overriding every caller’s Retry policy?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses the narrow, separable problem I raised in #4026: definitively-unrecoverable connection errors are retried with full backoff. The intentional handshake-phase retry behavior (clarified by @petyaslavova in that thread) is left completely untouched.
Since 7.1.0,
Connection.connect()wraps the entire connect + handshake in the retry policy. During socket establishment everyOSErroris re-wrapped into a genericConnectionError; because the policy's supported errors includeConnectionError, these are retried up toDEFAULT_RETRY_COUNT(10) times with exponential backoff — even for errors that can never succeed on retry, e.g. a Unix-domain-socketENOENT(the socket path simply doesn't exist). The wrap also discarded the originalerrno, so nothing downstream could distinguish a permanent failure from a transient one.Net effect:
Redis(unix_socket_path=..., socket_connect_timeout=2)against a missing socket spends seconds sleeping/retrying, and the common "try UDS, fall back to TCP" pattern becomes very slow.Change
Uses the existing
is_retryablehook inRetry.call_with_retrythatconnect()simply wasn't passing:OSError→ConnectionErrorwrap now usesraise ... from e, so the originalerrnosurvives for classification.is_retryablepredicate fromconnect()that returnsFalsefor a conservative set of unrecoverable socket errnos (currently justENOENT). Those fail fast; transient errors such asECONNREFUSEDduring server startup keep retrying exactly as before.Applied to both the sync (
redis/connection.py) and async (redis/asyncio/connection.py) stacks.Tests
test_connect_without_retry_on_unrecoverable_oserror(sync + async):ENOENT→ single attempt, originalOSErrorpreserved as__cause__.test_connect_retries_on_transient_oserror(sync + async):ECONNREFUSED→ still retried (3 attempts withretries=2).Open questions (carried over from #4026)
Kept deliberately conservative — happy to adjust to your preference:
ENOENTfor now — should it also includeEACCES/EAFNOSUPPORT/EPROTONOSUPPORT?socket_connect_timeoutas a total-time cap: left out of scope here (a broader change carrying a deadline across retries); could be a follow-up.Refs #4026.
Note
Low Risk
Narrow connect-path retry behavior change with conservative errno list and tests; handshake retry logic is unchanged.
Overview
Connect failures that cannot succeed on retry (e.g. missing Unix socket,
ENOENT) now fail immediately instead of burning the default retry count and exponential backoff.Sync and async
Connection.connect()passis_retryable=self._is_retryable_connect_errorinto the existingRetry.call_with_retryhook. The predicate treats errors as non-retryable when the raisedConnectionErrorchains anOSErrorwhoseerrnois in_UNRETRYABLE_CONNECT_ERRNOS(currentlyENOENTonly).ECONNREFUSEDand other transient errors keep retrying as before.OSErrorhandling during connect now usesraise ConnectionError(...) from eso the original errno is available on__cause__for that classification. Tests cover one-shot failure onENOENTand full retries onECONNREFUSED.Reviewed by Cursor Bugbot for commit 3459a64. Bugbot is set up for automated code reviews on this repo. Configure here.